Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

TestResults/runs/20260218-213838/20260218-213838-live-hero-helper.trx-2-2 (1)

2-2: ⚠️ Potential issue | 🟠 Major

Developer PII (username, machine name, and local absolute paths) committed to public repository.

The following attributes embed personal identifiable information that will be permanently stored in git history:

Attribute	Value
runUser (line 2)	DESKTOP-A60S9GP\Prekzursil
runDeploymentRoot (line 5)	Prekzursil_DESKTOP-A60S9GP_2026-02-18_23_39_54
storage (line 16)	full local path under c:\users\prekzursil\...
codeBase (line 18)	full local path under C:\Users\Prekzursil\...

If this artifact (and similar TRX files in this PR) must be committed, consider either:

1. Post-processing the TRX before committing to scrub machine-specific fields, or
2. Adding TestResults/runs/**/*.trx to .gitignore and committing only the aggregated live-validation-summary.json (which presumably does not carry these fields).

Also applies to: 5-5, 16-16, 18-18

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@TestResults/runs/20260218-213838/20260218-213838-live-hero-helper.trx` at
line 2, The TRX contains developer PII in attributes runUser, runDeploymentRoot,
storage, and codeBase; remove or neutralize those values in
TestResults/runs/20260218-213838/20260218-213838-live-hero-helper.trx (replace
with generic placeholders or strip the attributes) and re-commit the sanitized
TRX, and to prevent recurrence add an ignore rule for TRX artifacts (e.g.
TestResults/runs/**/*.trx) to .gitignore and stop tracking existing TRX files
(remove them from the index so they are not committed going forward).

TestResults/runs/20260218-213838/20260218-213838-live-tactical.trx-8-8 (1)

8-8: ⚠️ Potential issue | 🟠 Major

startTime/endTime timestamps are inconsistent with duration — live evidence artifact data integrity gap.

duration="00:00:34.2247037" (34.2 s) is correct and consistent with both the xUnit Finished log at 00:00:34.42 and the run-level start/finish wall clock (Lines 3, 35). However, startTime and endTime differ by only ~35 µs — essentially identical — because the xUnit VSTest adapter flushes per-test timestamps at result-collection time rather than at actual test start/end boundaries.

This means the per-test timestamps cannot be used to cross-correlate against the live-process diagnostics cited in the PR (selectedHostProcess, backendRouteDecision, capabilityProbeSnapshot, hookInstallReport). For an artifact that is the authoritative live-validation evidence, this reduces its forensic value. Consider enriching the test with explicit wall-clock markers logged to StdOut, or using a wrapper that captures OS-level start/end timestamps and injects them into the repro bundle rather than relying on VSTest's per-test timestamps.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@TestResults/runs/20260218-213838/20260218-213838-live-tactical.trx` at line
8, The per-test startTime/endTime written by the VSTest adapter are not true
wall-clock boundaries (they're flushed at result collection), so the test
SwfocTrainer.Tests.Profiles.LiveTacticalToggleWorkflowTests.Tactical_Toggles_Should_Execute_And_Revert_When_Tactical_Mode
must emit reliable OS-level timestamps: modify that test (or its test
fixture/wrapper) to capture high-resolution DateTime.UtcNow (or Stopwatch ticks)
at the actual test start and end and write them to StdOut (or into the test
result payload) so they become part of the run artifact; alternatively implement
a thin wrapper around the test execution that records OS timestamps and injects
them into the repro bundle/trace rather than relying on VSTest’s
startTime/endTime.

TestResults/runs/20260218-213838/20260218-213838-live-tactical.trx-2-2 (1)

2-2: ⚠️ Potential issue | 🟠 Major

PII (username + hostname) committed to repository.

runUser="DESKTOP-A60S9GP\Prekzursil" embeds the developer's Windows username and machine hostname directly in a committed artifact. Combined with the same values in computerName (Line 8) and the storage/codeBase paths (Lines 16–18), the full local identity is permanently written into Git history on a public repository.

Consider scrubbing or templating these fields before committing (e.g. replace with REDACTED or a CI-neutral placeholder), or generate the TRX in a sanitised form via a post-processing script in the tooling pipeline.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@TestResults/runs/20260218-213838/20260218-213838-live-tactical.trx` at line
2, The committed TRX embeds local PII in the TestRun element (attributes runUser
and computerName and paths in storage/codeBase); sanitize these fields by
replacing the values with a neutral placeholder (e.g., REDACTED or CI_MACHINE)
before committing or emitting the file, and add a post-processing sanitiser in
your test pipeline that locates the TestRun element and overwrites runUser,
computerName, storage and codeBase attributes (or removes them) so no local
username/hostname or local paths are written to Git history.

native/SwfocExtender.Overlay/include/swfoc_extender/overlay/OverlayState.hpp-11-11 (1)

11-11: ⚠️ Potential issue | 🟠 Major

visible_ is not thread-safe — use std::atomic<bool>.

In a game overlay, setVisible is typically driven by an input/hotkey thread while visible() is polled by the render thread. Accessing a plain bool across threads without synchronization is undefined behaviour in C++. The fix is a one-line change and doesn't affect the noexcept contract.

🔒 Proposed fix

+#include <atomic>
+
 namespace swfoc::extender::overlay {

 class OverlayState {
 public:
     void setVisible(bool visible) noexcept;
     bool visible() const noexcept;

 private:
-    bool visible_ {false};
+    std::atomic<bool> visible_ {false};
 };

With std::atomic<bool>, the setter/getter implementations in OverlayState.cpp remain correct as-is — operator= and implicit operator bool use the default seq_cst ordering, which is safe for a simple visibility flag. If relaxed ordering is acceptable for performance, use store/load explicitly.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@native/SwfocExtender.Overlay/include/swfoc_extender/overlay/OverlayState.hpp`
at line 11, The member visible_ in class OverlayState is accessed from different
threads and must be made thread-safe: change its type from bool to
std::atomic<bool> (add `#include` <atomic>) so reads/writes are atomic; no changes
needed to the existing setVisible / visible methods in OverlayState.cpp since
atomic<bool>'s default operations provide the required ordering (or you can use
store/load with an explicit memory_order if you prefer).

TestResults/runs/20260218-213838/launch-context-fixture.json-4-4 (1)

4-4: ⚠️ Potential issue | 🟠 Major

Developer's local username and machine path committed to source control; path also has a double-escaping bug.

Two distinct issues on this line:

1. PII / path leakage (major): The detail field contains a hardcoded local filesystem path that embeds the developer's OS username: C:\\Users\\Prekzursil\\AppData\\Local\\.... Committing personal local paths to source control leaks filesystem/username information and becomes a permanent part of git history. The value should be redacted or replaced with a sanitised placeholder such as <python-executable> or a path relative to a well-known env var (e.g., %LOCALAPPDATA%\\...).

2. Malformed path from double-escaping (minor): After JSON string parsing, \\ becomes \ and \\\\ becomes \\. The segment Programs\\\\Python\\\\Python312\\\\python.exe decodes to Programs\\Python\\Python312\\python.exe — literal double-backslashes — which is not a valid Windows path. The first half of the path uses correct single \\ escaping while the second half uses \\\\. This is a classic PowerShell ConvertTo-Json artefact when the input string was already escaped. The path serialisation should use a single level of JSON escaping throughout.

🛡️ Proposed fix — redact PII and fix escaping

-    "detail":  "python produced no output. executable: C:\\Users\\Prekzursil\\AppData\\Local\\Programs\\\\Python\\\\Python312\\\\python.exe",
+    "detail":  "python produced no output. executable: %LOCALAPPDATA%\\Programs\\Python\\Python312\\python.exe",

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@TestResults/runs/20260218-213838/launch-context-fixture.json` at line 4, The
"detail" string in the JSON contains a developer's local path (PII) and a
double-escaping artifact; update the serialization that produces the "detail"
field so it emits a sanitized value (e.g., "<python-executable>" or a
platform-env-based placeholder like "%LOCALAPPDATA%\\...") instead of the full
C:\\Users\\... path, and ensure JSON string escaping is performed only once so
backslashes are single-escaped (use the actual path string not an
already-escaped string before calling JSON serialization). Target the code that
sets the "detail" field (the producer that writes launch-context-fixture.json /
the value assigned to "detail") and replace hardcoded local paths with a
redaction/placeholder and fix the escaping pipeline to avoid double-escaped
backslashes.

native/SwfocExtender.Core/include/swfoc_extender/core/HookLifecycleManager.hpp-20-31 (1)

20-31: ⚠️ Potential issue | 🟠 Major

HookLifecycleManager has no thread-safety guarantees — concurrent access to hooks_ would be undefined behaviour.

markInstalled, markFailed, markRolledBack, and get all read/write a shared std::unordered_map with no synchronisation. If called from more than one thread, the result is a data race and UB.

Add protection using std::shared_mutex (shared lock for get, exclusive lock for the three mutators), or document explicitly that the class must be accessed from a single thread only.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@native/SwfocExtender.Core/include/swfoc_extender/core/HookLifecycleManager.hpp`
around lines 20 - 31, HookLifecycleManager currently accesses the shared
unordered_map hooks_ from multiple methods without synchronization; add a
std::shared_mutex member (e.g., mutex_) and protect access: use
std::unique_lock<std::shared_mutex> in mutators markInstalled, markFailed,
markRolledBack and use std::shared_lock<std::shared_mutex> in get to allow
concurrent reads; also include the appropriate header(s) (<shared_mutex>) and
ensure the lock objects are created at the start of each method to guard hooks_.

tools/research/extract-pe-metadata.ps1-10-12 (2)

10-12: ⚠️ Potential issue | 🟠 Major

Failure condition missing explicit classification code.

As per coding guidelines, "Failure conditions must use explicit classification codes." The bare throw "Module not found: ..." gives no structured code for callers or CI pipelines to key on.

🔧 Proposed fix

-    throw "Module not found: $moduleFullPath"
+    throw "[MODULE_NOT_FOUND] Module not found: $moduleFullPath"

As per coding guidelines: "Failure conditions must use explicit classification codes."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tools/research/extract-pe-metadata.ps1` around lines 10 - 12, Replace the
bare throw in the module existence check with a failure that includes an
explicit classification code so callers/CI can parse it: when Test-Path for
$moduleFullPath fails, throw or write an error that prefixes a machine-readable
code (e.g. ERR_MODULE_NOT_FOUND or E_MODULE_NOT_FOUND) along with the
descriptive message and ensure the thrown object/message includes both the code
and $moduleFullPath (update the throw/writing location around the Test-Path
check referencing $moduleFullPath).

---

10-12: ⚠️ Potential issue | 🟠 Major

Failure condition missing explicit classification code.

The throw emits a plain prose string. As per coding guidelines, "Failure conditions must use explicit classification codes." Downstream scripts and CI pipelines cannot key on a structured code for routing or suppression.

🔧 Proposed fix

-    throw "Module not found: $moduleFullPath"
+    throw "[MODULE_NOT_FOUND] Module not found: $moduleFullPath"

As per coding guidelines: "Failure conditions must use explicit classification codes."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tools/research/extract-pe-metadata.ps1` around lines 10 - 12, Replace the
plain string throw with a structured failure that includes an explicit
classification code; in the file's existence check where the script currently
does throw "Module not found: $moduleFullPath", instead throw an exception whose
message begins with a stable classification token (e.g. "MODULE_NOT_FOUND") or
construct an ErrorRecord/Exception with a Code-like field, include
$moduleFullPath in the message, and throw that object so downstream tooling can
reliably parse the classification (locate the throw that references
$moduleFullPath in extract-pe-metadata.ps1 and update it accordingly).

tools/research/extract-pe-metadata.ps1-1-38 (2)

1-38: ⚠️ Potential issue | 🟠 Major

No deterministic fixture or smoke command included.

As per coding guidelines, "Every tooling script change must include a deterministic fixture or smoke command." The script ships with no inline example, no sibling fixture file, and no CI smoke step that invokes it against a known-good binary and asserts the output schema. Add a smoke invocation at minimum as a comment block, or a tests/fixtures/smoke-extract-pe-metadata.ps1:

# Smoke example — run from repo root against a committed fixture binary:
# .\tools\research\extract-pe-metadata.ps1 `
#     -ModulePath  "tests/fixtures/sample.exe" `
#     -OutputPath  "TestResults/runs/smoke/sample.metadata.json"
# $out = Get-Content "TestResults/runs/smoke/sample.metadata.json" | ConvertFrom-Json
# if ($out.schemaVersion -ne "1.0")  { throw "[SMOKE_SCHEMA_MISMATCH] schemaVersion unexpected" }
# if (-not $out.fileVersion)         { throw "[SMOKE_FIELD_EMPTY] fileVersion is empty" }

As per coding guidelines: "Every tooling script change must include a deterministic fixture or smoke command."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tools/research/extract-pe-metadata.ps1` around lines 1 - 38, The script
tools/research/extract-pe-metadata.ps1 lacks the required deterministic
smoke/fixture; add a minimal reproducible smoke invocation and assertion either
as a commented example at top of the script or as a separate
tests/fixtures/smoke-extract-pe-metadata.ps1 that calls the script with
-ModulePath and -OutputPath, reads the produced JSON and asserts
result.schemaVersion == "1.0" and that result.fileVersion (or another required
field) is non-empty; reference the param names ModulePath and OutputPath, the
output file schemaVersion field, and the conversion step (ConvertFrom-Json) in
the assertions so CI or a reviewer can run the smoke and validate the schema.

---

1-38: ⚠️ Potential issue | 🟠 Major

No deterministic fixture or smoke command included.

As per coding guidelines, every tooling script change must include a deterministic fixture or smoke command. Neither the script nor its diff adds an inline example invocation, a sibling fixture file, or a CI smoke step that validates the script's output against a known-good artifact.

A minimal approach is a smoke invocation comment block at the bottom, or a companion tests/fixtures/extract-pe-metadata.smoke.ps1:

# Smoke example (run from repo root, output verified against schema):
# .\tools\research\extract-pe-metadata.ps1 `
#   -ModulePath "tests/fixtures/sample.exe" `
#   -OutputPath "TestResults/runs/smoke/sample.metadata.json"
# $out = Get-Content "TestResults/runs/smoke/sample.metadata.json" | ConvertFrom-Json
# if ($out.schemaVersion -ne "1.0") { throw "[SMOKE_FAIL] schemaVersion mismatch" }

As per coding guidelines: "Every tooling script change must include a deterministic fixture or smoke command."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tools/research/extract-pe-metadata.ps1` around lines 1 - 38, Add a
deterministic smoke invocation for the extract-pe-metadata.ps1 script: include a
commented example block at the bottom of extract-pe-metadata.ps1 showing how to
run Extract-PE-Metadata (ModulePath and OutputPath parameters), then read and
validate the produced JSON (ConvertFrom-Json) and assert schemaVersion == "1.0"
(throw on mismatch); alternatively add a small companion fixture file
tests/fixtures/extract-pe-metadata.smoke.ps1 that performs the same
deterministic run against a committed sample (e.g., tests/fixtures/sample.exe)
and exits non-zero on failure so CI or reviewers can run the smoke test.

tools/research/extract-pe-metadata.ps1-1-4 (2)

1-4: ⚠️ Potential issue | 🟠 Major

Missing script help block and Windows-only assumption documentation.

[System.Diagnostics.FileVersionInfo]::GetVersionInfo only yields meaningful ProductName, FileVersion, OriginalFilename, etc. for PE (Windows) binaries. Running this against a non-PE file silently emits null/empty fields with no error. As per coding guidelines, "Windows live target assumptions must be kept explicit in documentation and script help." Add at least a #Requires comment or a comment-based help block with a .NOTES section calling this out, and add [CmdletBinding()] to enable -Verbose.

📝 Proposed minimal help block

+<#
+.SYNOPSIS
+    Extracts PE (Portable Executable) metadata from a Windows binary and writes it as structured JSON.
+
+.DESCRIPTION
+    Reads FileVersionInfo and file-system properties from a Windows PE binary and emits a
+    schema-versioned JSON artifact. Only meaningful on PE/Windows targets; non-PE binaries
+    will yield empty version fields without error.
+
+.PARAMETER ModulePath
+    Path to the target PE module (e.g. StarWarsG.exe).
+
+.PARAMETER OutputPath
+    Optional. Destination JSON path. Defaults to <ModulePath-without-extension>.metadata.json.
+
+.NOTES
+    WINDOWS-ONLY ASSUMPTION: FileVersionInfo fields are PE-specific. Run on Windows hosts only.
+    Output isolation: pass an explicit -OutputPath rooted under TestResults/runs/<runId>/ for
+    per-run artifact tracking.
+#>
+[CmdletBinding()]
 param(
     [Parameter(Mandatory = $true)][string]$ModulePath,
     [string]$OutputPath
 )

As per coding guidelines: "Windows live target assumptions must be kept explicit in documentation and script help."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tools/research/extract-pe-metadata.ps1` around lines 1 - 4, Add a
comment-based help block and make the Windows-only assumption explicit, and
enable -Verbose: add [CmdletBinding()] above the param(...) block to enable
-Verbose, then add a comment-based help block (Synopsis, Description, Parameters
for ModulePath and OutputPath, and a .NOTES section) that calls out that this
script targets Windows PE files and that
[System.Diagnostics.FileVersionInfo]::GetVersionInfo will return null/empty for
non-PE files; alternatively include a top-level `#Requires` comment indicating
Windows, and mention the ModulePath/OutputPath parameters and the reliance on
[System.Diagnostics.FileVersionInfo]::GetVersionInfo in the .NOTES so callers
know the Windows-only assumption.

---

1-4: ⚠️ Potential issue | 🟠 Major

Add a script help block documenting the Windows/PE-only assumption.

[System.Diagnostics.FileVersionInfo]::GetVersionInfo fields (ProductName, FileVersion, OriginalFilename, etc.) are only meaningful for PE (Windows) binaries. A non-PE binary silently emits null/empty fields without error. There is also no [CmdletBinding()], so common parameters like -Verbose, -ErrorAction, and -WhatIf are unavailable. As per coding guidelines, "Windows live target assumptions must be kept explicit in documentation and script help."

📝 Proposed help block + CmdletBinding

+<#
+.SYNOPSIS
+    Extracts PE (Portable Executable) metadata from a Windows binary and writes it as structured JSON.
+
+.DESCRIPTION
+    Reads FileVersionInfo and file-system properties from a Windows PE binary and emits a
+    schema-versioned JSON artifact (schemaVersion 1.0).
+    NOTE: Windows PE/Windows-only assumption — FileVersionInfo fields are PE-specific. Non-PE
+    binaries will yield empty version fields without error. Must be run on a Windows host.
+
+.PARAMETER ModulePath
+    Path to the target PE module (e.g. StarWarsG.exe).
+
+.PARAMETER OutputPath
+    Optional. Destination JSON path. Defaults to <ModulePath-without-extension>.metadata.json.
+    For per-run artifact tracking, supply a path rooted under TestResults/runs/<runId>/.
+
+.NOTES
+    Output isolation: the default path places the artifact alongside the source binary.
+    Pass an explicit -OutputPath under TestResults/runs/<runId>/ when used in validation runs.
+#>
+[CmdletBinding()]
 param(
     [Parameter(Mandatory = $true)][string]$ModulePath,
     [string]$OutputPath
 )

As per coding guidelines: "Windows live target assumptions must be kept explicit in documentation and script help."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tools/research/extract-pe-metadata.ps1` around lines 1 - 4, Add a PowerShell
help block and CmdletBinding to the script: document that the script operates on
Windows PE binaries only and that
[System.Diagnostics.FileVersionInfo]::GetVersionInfo fields (ProductName,
FileVersion, OriginalFilename, etc.) will be null/empty for non-PE files;
include examples for the mandatory parameter ModulePath and optional OutputPath,
and add [CmdletBinding()] at the top so common parameters like -Verbose,
-ErrorAction, and -WhatIf are available; ensure the help block clearly states
the Windows/PE-only assumption and expected behaviors when non-PE input is
provided.

tools/research/extract-pe-metadata.ps1-37-37 (2)

37-37: ⚠️ Potential issue | 🟠 Major

Set-Content -Encoding UTF8 writes a BOM on Windows PowerShell 5.x, breaking JSON consumers.

The official Microsoft documentation confirms that in PowerShell 5.1, UTF8 encoding means "UTF-8 (with BOM)". The utf8NoBOM value is not available in PS 5.1 — it was introduced in PowerShell 6. A BOM-prefixed JSON file can silently fail or be misparsed by jq, ConvertFrom-Json in some hosts, and any byte-level JSON parser that doesn't strip the BOM preamble. The safe cross-version fix is [System.IO.File]::WriteAllText with an explicit no-BOM encoding instance.

🔧 Proposed fix

-($result | ConvertTo-Json -Depth 6) | Set-Content -Path $OutputPath -Encoding UTF8
+$json = $result | ConvertTo-Json -Depth 6
+[System.IO.File]::WriteAllText($OutputPath, $json, [System.Text.UTF8Encoding]::new($false))

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tools/research/extract-pe-metadata.ps1` at line 37, The current write uses
Set-Content with -Encoding UTF8 which produces a BOM on PowerShell 5.x; replace
that write with a call to [System.IO.File]::WriteAllText using the JSON string
produced by ConvertTo-Json ($result | ConvertTo-Json -Depth 6) and an explicit
no-BOM encoding instance (e.g. New-Object System.Text.UTF8Encoding($false)) so
the file at $OutputPath is written as UTF‑8 without a BOM; update the code that
currently calls Set-Content to build the JSON string then call WriteAllText with
the no‑BOM encoding.

---

37-37: ⚠️ Potential issue | 🟠 Major

Set-Content -Encoding UTF8 writes a UTF-8 BOM on Windows PowerShell 5.1, breaking JSON consumers downstream.

Windows PowerShell 5.1 writes UTF-8 with BOM (bytes EF BB BF) when using -Encoding UTF8, while PowerShell 7+ defaults to no BOM. JSON parsers (jq, ConvertFrom-Json, Node.js) can fail or misparse with a leading BOM. Use [System.IO.File]::WriteAllText with [System.Text.UTF8Encoding]::new($false) for cross-version correctness and JSON schema compliance.

🔧 Proposed fix

-($result | ConvertTo-Json -Depth 6) | Set-Content -Path $OutputPath -Encoding UTF8
+$json = $result | ConvertTo-Json -Depth 6
+[System.IO.File]::WriteAllText($OutputPath, $json, [System.Text.UTF8Encoding]::new($false))

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tools/research/extract-pe-metadata.ps1` at line 37, The current use of
Set-Content with -Encoding UTF8 writes a BOM on Windows PowerShell 5.1; replace
the Set-Content call so the JSON produced by ConvertTo-Json (using $result and
$OutputPath) is written without a BOM by calling the .NET API to write the
string (e.g., use [System.IO.File]::WriteAllText) and pass an explicit UTF8
encoding instance that disables the BOM (e.g.,
[System.Text.UTF8Encoding]::new($false)); update the line that references
Set-Content to use this WriteAllText approach so downstream JSON consumers
aren’t broken.

src/SwfocTrainer.Profiles/Services/FileSystemProfileRepository.cs-154-158 (1)

154-158: ⚠️ Potential issue | 🟠 Major

BackendPreference / HostPreference fallback to parent is unreachable due to non-null defaults.

As noted in ProfileModels.cs, the record defaults ("auto", "starwarsg_preferred") mean child.BackendPreference and child.HostPreference are never null/whitespace after deserialization, so lines 155 and 157 always select the child value. The parent's values can never propagate.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/SwfocTrainer.Profiles/Services/FileSystemProfileRepository.cs` around
lines 154 - 158, The fallback logic for BackendPreference and HostPreference in
FileSystemProfileRepository.cs never uses the parent because
child.BackendPreference and child.HostPreference are initialized to non-empty
defaults; update the ternaries to treat those default sentinel values as "not
set". Specifically, when constructing the merged profile, replace the checks
string.IsNullOrWhiteSpace(child.BackendPreference) and
string.IsNullOrWhiteSpace(child.HostPreference) with comparisons that detect the
record defaults (e.g., child.BackendPreference ==
ProfileDefaults.BackendPreferenceDefault or child.BackendPreference == "auto",
and child.HostPreference == ProfileDefaults.HostPreferenceDefault or
child.HostPreference == "starwarsg_preferred") and fall back to
parent.BackendPreference / parent.HostPreference when the child equals the
sentinel default.

src/SwfocTrainer.Core/Models/ProfileModels.cs-46-50 (1)

46-50: ⚠️ Potential issue | 🟠 Major

Default values for BackendPreference and HostPreference prevent child profiles from inheriting parent values.

The merge logic (lines 155–157 of FileSystemProfileRepository) uses string.IsNullOrWhiteSpace() to decide whether to use a child's value or fall back to the parent's. Since BackendPreference defaults to "auto" and HostPreference defaults to "starwarsg_preferred", any child profile omitting these fields will be deserialized with the default non-empty strings. The merge will then treat these defaults as explicitly set values, making it impossible to inherit from the parent.

While all current profiles explicitly set both fields (no inheritance is attempted), this is a fragile design that prevents inheritance semantics from working correctly. Consider either:

1. Using null as the default and applying "auto" / "starwarsg_preferred" as runtime defaults downstream, or
2. Updating the merge logic to check if the child value equals the record default before deciding to fall back to the parent.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/SwfocTrainer.Core/Models/ProfileModels.cs` around lines 46 - 50, The
record defaults for BackendPreference ("auto") and HostPreference
("starwarsg_preferred") on ProfileModels.cs cause child profiles to appear
explicitly set and block inheritance in FileSystemProfileRepository's merge
logic (used around the merge method that checks string.IsNullOrWhiteSpace on the
child). Fix by making those properties nullable defaults (BackendPreference =
null, HostPreference = null) so omitted fields deserialize as null and
inheritance works, and ensure any runtime use that needs
"auto"/"starwarsg_preferred" applies those as runtime defaults; alternatively,
if you prefer code-change in merging, update the merge in
FileSystemProfileRepository to treat the record default values as "unset" when
deciding to fall back to parent (i.e., check for null or equal to the record
default before using child value).

src/SwfocTrainer.Runtime/Services/RuntimeModeProbeResolver.cs-101-114 (1)

101-114: ⚠️ Potential issue | 🟠 Major

CountSignals uses a negative health-status filter that admits degraded symbols as valid signals.

The condition skips a symbol only if its HealthStatus is Unresolved. SymbolHealthStatus has three values: Healthy, Degraded, and Unresolved. The current filter allows Degraded symbols to be counted, but degraded symbols should not contribute to mode determination. The stated intent is to count only symbols with a healthy status. Use a positive guard instead:

🛡️ Proposed fix

-if (info.Address == nint.Zero || info.HealthStatus == SymbolHealthStatus.Unresolved)
+if (info.Address == nint.Zero || info.HealthStatus != SymbolHealthStatus.Healthy)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/SwfocTrainer.Runtime/Services/RuntimeModeProbeResolver.cs` around lines
101 - 114, CountSignals currently treats any symbol that is not Unresolved as
valid, which incorrectly counts Degraded symbols; update the guard in
CountSignals to only accept symbols whose info.HealthStatus is
SymbolHealthStatus.Healthy (and still ensure info.Address != nint.Zero and info
!= null). Locate the method CountSignals and replace the negative health check
against Unresolved with a positive check for SymbolHealthStatus.Healthy so only
healthy symbols contribute to the count.

src/SwfocTrainer.Runtime/Services/NamedPipeExtenderBackend.cs-225-258 (1)

225-258: ⚠️ Potential issue | 🟠 Major

Thread.Sleep(200) inside lock(HostSync) blocks the thread pool and holds the lock.

Two compounding problems:

1. Lock held for 200 ms: any concurrent caller racing to TryStartBridgeHostProcess stalls on lock acquisition for the entire sleep duration, not just for the process-start check.
2. Async context: TryStartBridgeHostProcess is reached from an async method (SendAsync). The .Sleep pins the current thread pool thread instead of yielding, risking pool starvation under sustained concurrent usage.

Minimum fix — move the wait outside the lock so other callers can read HasExited immediately:

🐛 Proposed fix (minimum — move sleep outside lock)

-        try
-        {
-            _bridgeHostProcess = Process.Start(new ProcessStartInfo
-            {
-                FileName = hostPath,
-                WorkingDirectory = Path.GetDirectoryName(hostPath) ?? AppContext.BaseDirectory,
-                UseShellExecute = false,
-                CreateNoWindow = true
-            });
-            Thread.Sleep(200);
-            return _bridgeHostProcess is not null && !_bridgeHostProcess.HasExited;
-        }
+        Process? started = null;
+        try
+        {
+            _bridgeHostProcess = Process.Start(new ProcessStartInfo
+            {
+                FileName = hostPath,
+                WorkingDirectory = Path.GetDirectoryName(hostPath) ?? AppContext.BaseDirectory,
+                UseShellExecute = false,
+                CreateNoWindow = true
+            });
+            started = _bridgeHostProcess;
+        }
         catch
         {
             _bridgeHostProcess = null;
             return false;
         }
+    }
+    // Wait for process startup *outside* the lock so concurrent callers are not blocked.
+    if (started is not null)
+    {
+        Thread.Sleep(200);
     }
+    return started is not null && !started.HasExited;
 }

Preferred fix: make TryStartBridgeHostProcess async, replace Thread.Sleep with await Task.Delay(200), and replace the lock with a SemaphoreSlim.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/SwfocTrainer.Runtime/Services/NamedPipeExtenderBackend.cs` around lines
225 - 258, TryStartBridgeHostProcess currently holds lock(HostSync) while
calling Thread.Sleep(200), which blocks thread-pool threads and prevents
concurrent callers (e.g., SendAsync) from observing
_bridgeHostProcess.HasExited; change the method to an async implementation:
replace the lock(HostSync) with a SemaphoreSlim (e.g., _hostSemaphore) awaited
with WaitAsync/Release, replace Thread.Sleep(200) with await Task.Delay(200)
(moved outside the critical section if you keep the lock approach), and ensure
all call sites (including SendAsync) await the async TryStartBridgeHostProcess;
ensure _bridgeHostProcess is still checked/assigned inside the
semaphore-protected section and the post-delay HasExited check happens after the
delay without holding the semaphore longer than necessary.

tools/research/run-capability-intel.ps1-16-18 (1)

16-18: ⚠️ Potential issue | 🟠 Major

Orchestrator output root TestResults/research/<RunId>/ violates the per-run isolation mandate.

This is the top-level orchestrator for the capability intel workflow and sets the $OutputRoot that flows into all three sub-scripts. Fixing it here corrects the path for all artifacts produced by the pipeline (fingerprint.json, signature-pack.json, analysis-notes.md) in a single change.

🐛 Proposed fix

 if ([string]::IsNullOrWhiteSpace($OutputRoot)) {
-    $OutputRoot = Join-Path $repoRoot "TestResults/research/$RunId"
+    $OutputRoot = Join-Path $repoRoot "TestResults/runs/$RunId/research"
 }

As per coding guidelines: "Per-run output isolation must be maintained under TestResults/runs/<runId>/ directory structure".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tools/research/run-capability-intel.ps1` around lines 16 - 18, The current
orchestration sets $OutputRoot to TestResults/research/$RunId which violates
per-run isolation; change the Join-Path call that assigns $OutputRoot (when
[string]::IsNullOrWhiteSpace($OutputRoot)) to use the mandated structure
"TestResults/runs/<RunId>" (e.g., Join-Path $repoRoot "TestResults/runs/$RunId")
so all downstream artifacts (fingerprint.json, signature-pack.json,
analysis-notes.md) produced via $OutputRoot are placed under the per-run
isolated directory; ensure $RunId is used exactly and no other references to
"research" remain in this assignment.

tools/research/generate-signature-candidates.ps1-29-32 (1)

29-32: ⚠️ Potential issue | 🟠 Major

Default output path TestResults/research/<RunId>/ violates the per-run isolation mandate.

The coding guideline requires all per-run output under TestResults/runs/<runId>/. This script (and run-capability-intel.ps1) write to a parallel TestResults/research/ subtree, meaning downstream consumers that expect artifacts under the canonical path won't find them.

🐛 Proposed fix

 if ([string]::IsNullOrWhiteSpace($OutputPath)) {
-    $outputDir = Join-Path $repoRoot "TestResults/research/$RunId"
+    $outputDir = Join-Path $repoRoot "TestResults/runs/$RunId/research"
     New-Item -ItemType Directory -Path $outputDir -Force | Out-Null
     $OutputPath = Join-Path $outputDir "signature-pack.json"
 } else {

As per coding guidelines: "Per-run output isolation must be maintained under TestResults/runs/<runId>/ directory structure".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tools/research/generate-signature-candidates.ps1` around lines 29 - 32, The
script currently builds a default per-run output directory using
TestResults/research/$RunId which violates the per-run isolation mandate; update
the path construction so $outputDir uses "TestResults/runs/$RunId" instead of
"TestResults/research/$RunId" (i.e., change the Join-Path call that combines
$repoRoot and "TestResults/research/$RunId" to use "TestResults/runs/$RunId")
and leave the subsequent New-Item and $OutputPath assignment (Join-Path for
"signature-pack.json") unchanged so artifacts are created under the canonical
per-run tree; ensure references to $OutputPath/$outputDir in this script (and
mirror the same change in run-capability-intel.ps1) are consistent.